Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-56935] Multibranch pipeline orphaned item strategy can't be set to 0 #199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[JENKINS-56935] Multibranch pipeline orphaned item strategy can't be set to 0 #199

wants to merge 1 commit into from

Conversation

ericcitaire
Copy link

@ericcitaire ericcitaire commented Nov 10, 2021 •
edited
Loading

See JENKINS-56935.

image

Proposed changelog entries

  • Fixes JENKINS-56935

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change).
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests

@ericcitaire
Copy link
Author

Can I have feedback on this please ?

ping @fcojfernandez @bitwiseman @alecharp

@fcojfernandez fcojfernandez requested a review from a team December 23, 2021 08:05
jtnord
jtnord requested changes Jan 4, 2022
View reviewed changes
Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be incorrect as the code is setting the internal value to -1 when the value is empty. by allowing 0 the internal field would no longer be using -1 and the quick deletion would no longer work.

If the behaviour is unclear I would suggest a change to the inline help and documentation rather than code

@jtnord jtnord changed the title Fixes JENKINS-56935 [JENKINS-56935] Multibranch pipeline orphaned item strategy can't be set to 0 Jan 4, 2022
@ericcitaire
Copy link
Author

this would be incorrect as the code is setting the internal value to -1 when the value is empty. by allowing 0 the internal field would no longer be using -1 and the quick deletion would no longer work.

If the behaviour is unclear I would suggest a change to the inline help and documentation rather than code

Yes, the behaviour is unclear.
From what I observe, my understanding is that an empty value or -1 basically means ignore this criteria, while a 0 value means don't keep anything.

Also, this is just a matter of UX because the 0 value is actually accepted, despite the displayed error message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtnord jtnord Awaiting requested review from jtnord

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ericcitaire @jtnord